Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add tables property to base model class #502

Merged
merged 7 commits into from
Sep 7, 2023
Merged

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Sep 6, 2023

Issue addressed

Fixes #377

Explanation

Copied and extended the code from Wflow as referenced in the issue. It seemed like a good idea to support more file types than just csv, so I implemented so common ones, but others can easily be added if necessary.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@savente93 savente93 marked this pull request as ready for review September 6, 2023 11:10
Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @savente93! A couple of comments to discuss

  • You deviated from the approach of other model properties by not implementing a dynamic (not sure this is the right terminology) public tables property and initiating the internal _tables property with None. Using a dynamic tables property allows to trigger reading upon first call (i.e. when the _tables property is still None; this should also be changed). See e.g. Model.maps property.
  • You also implemented a different get methods and an iter methods which we don't have for other properties. I think we can keep those but these should point to the tables instead of _tables property to trigger the reading.
  • I'm also not sure if we need to support different drivers in the read and write methods here. Most models will probably use a custom format and hence overwrite the read and write methods. Thus writing to different table formats like excel and/or parquet will likely not be used. I suggest to limit it to a single csv format similar to the other properties to keep thinks simple on our side.

Copy link
Contributor

@DirkEilander DirkEilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some final changes to

  1. make the method between properties more consistent, i.e., order of arguments in set_tables; usage of single set_tables method rather than singular and plural; and fn argument in read and write.
  2. add tables to default read and write components all (sub)model classes.
  3. added tables to append test

@savente93 savente93 merged commit 713d6e3 into main Sep 7, 2023
7 checks passed
@savente93 savente93 deleted the model_tabular_data branch September 7, 2023 18:27
@savente93
Copy link
Contributor Author

@DirkEilander Thanks for fixing the tests :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Add a new tables object to Model API
2 participants